feat: multi-page structure with React Router #12
Conversation
BREAKING CHANGE: Some tests need to be changes and app needs to be tested more
There was a problem hiding this comment.
A couple of things I found while testing.
-
If you have a filter on, and then click on a book, it doesn't find the book, and you get the "Book not found" error.
-
The filters no longer use the debounce mechanism.
-
This might be fixed when debounce arrives, but currently the back button is overwritten by the searchbar. That means that right now, if i enter in something in a filter, eg., "sanderson", every back button i press just removes one letter. Debounce will make this not appear so freqently, but still would be nice to fix.
Co-authored-by: Johannes Horn <johahor@stud.ntnu.no>
Co-authored-by: Johannes Horn <johahor@stud.ntnu.no>
|
Fixed the issues you've mentioned: Browsing books with filters should work, filters use debouncing and the usage of buttons inside links is fixed. |
There was a problem hiding this comment.
While testing, I found another issue. When selecting the first book on page 2 of the grid and then clicking the "Previous" button on the selectedBook page, the button still triggers even though it looks disabled.
It also looks like a few of the tests are failing.
I tried to reproduce it, and it's hard, pointing towards that this might not be necessary to keep working on, but i'll still try to explain. If you go into one of the filter, type in "sand", wait a second so the debounce settles, then complete the word so it types out "sanderson". That is now registered as two different paths in the browser stacked on top of each other, meaning that if you click on the back button, nothing happens. I'm not sure what happened since last time i tested, but now when i tested it is more hidden, but still there. |
|
I've taken a look at both of the issues you guys addressed. The back navigation was more tricky, but it seems like it's fine now. |
Checked. Now the url matches what the input field does. Nothing blocking merge anymore, now it's more a decision based on taste of the solution. Could argue that pressing back should just wipe the whole field, but personally i think this is a good solution. Good work on the fix. |
This PR is a refactor of the application to use multiple pages instead of sending a ton of props and callbacks back and forth.
We use a separate URL
/book/:bookIdfor each book, wherebookIdis dynamic (the OpenLibrary ID). Some components have been removed and are the complete page instead, for exampleBookDetailsis the book page itself.To be merged after #11